feat(c/sedona-extension): Add FFI table provider, exec plan, and synchronous stream exchange#1004
feat(c/sedona-extension): Add FFI table provider, exec plan, and synchronous stream exchange#1004paleolimbot wants to merge 58 commits into
Conversation
a7ef707 to
1fdef0c
Compare
This reverts commit 56f2ed6.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Sedona C-extension FFI layer for exporting/importing TableProvider and ExecutionPlan objects across process/runtime boundaries (Python/R/ADBC), enabling cross-SedonaDB-context DataFrame round-tripping while preserving DataFusion optimization opportunities (e.g., projection pushdown).
Changes:
- Add a new
sedona-extensioncrate implementing ABI-stable C interfaces for table providers, execution plans, expressions, and Arrow stream utilities. - Replace the previous Rust/Python stream-reader utilities with a shared
StreamingRecordBatchReaderimplementation (with optional cancellation / empty-batch skipping). - Update Python/R/ADBC integrations to export/import providers via
__sedonadb_table_provider__and add Python FFI round-trip tests.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/sedona/src/reader.rs | Removes the old SedonaStreamReader implementation. |
| rust/sedona/src/lib.rs | Stops exporting the removed reader module. |
| rust/sedona/src/dataframe.rs | Adds a Sedona DataFrame extension trait + write options (currently duplicates context.rs). |
| rust/sedona-adbc/src/statement.rs | Switches ADBC streaming export to StreamingRecordBatchReader. |
| rust/sedona-adbc/Cargo.toml | Adds sedona-extension dependency. |
| r/sedonadb/src/rust/src/dataframe.rs | Switches R streaming export to StreamingRecordBatchReader. |
| r/sedonadb/src/rust/Cargo.toml | Adds sedona-extension dependency. |
| python/sedonadb/tests/test_dataframe.py | Updates comment to new __sedonadb_table_provider__ interface name. |
| python/sedonadb/tests/test_dataframe_ffi.py | Adds end-to-end Python tests validating FFI DataFrame round-trips. |
| python/sedonadb/src/runtime.rs | Removes now-unused Rust-side “wait for future” helper. |
| python/sedonadb/src/reader.rs | Replaces Python stream reader with new_py_streaming_reader using cancellation-aware StreamingRecordBatchReader. |
| python/sedonadb/src/import_from.rs | Switches provider import to new Sedona FFI table provider capsule type. |
| python/sedonadb/src/dataframe.rs | Exports __sedonadb_table_provider__ via sedona-extension’s exported provider implementation. |
| python/sedonadb/python/sedonadb/dataframe.py | Renames Python-side provider hook to __sedonadb_table_provider__ and updates docs/comments. |
| python/sedonadb/Cargo.toml | Adds sedona-extension dependency. |
| Cargo.lock | Records new crate dependencies for sedona-extension usage. |
| c/sedona-extension/src/utils.rs | Adds Arrow stream/property utilities + StreamingRecordBatchReader implementation and tests. |
| c/sedona-extension/src/table_provider.rs | Adds exported/imported TableProvider FFI wrappers and tests. |
| c/sedona-extension/src/execution_plan.rs | Adds exported/imported ExecutionPlan FFI wrappers and tests. |
| c/sedona-extension/src/expr.rs | Adds exported/imported expression FFI wrappers and tests. |
| c/sedona-extension/src/sedona_extension.h | Extends the C header with new ABI structs and callbacks. |
| c/sedona-extension/src/lib.rs | Exposes new execution_plan, expr, table_provider, and utils modules. |
| c/sedona-extension/src/extension.rs | Adds Rust FFI struct definitions for new ABI types (error/expr/plan/provider). |
| c/sedona-extension/Cargo.toml | Adds dependencies needed for new FFI/provider/plan implementations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| struct SedonaCError* err); | ||
|
|
||
| /// \brief Reserved for future use (must be NULL). | ||
| void* reserved; |
There was a problem hiding this comment.
these are advanced tactics heretofore unknown to me
There was a problem hiding this comment.
Evolving an ABI safely without a crash as the failure mode is hard. We might want to do something else (just a brand new struct with a different name), but we can't go back and add a member to the struct and a pointer gives the most future options.
There was a problem hiding this comment.
Should we be versioning the interface like a v1 suffix on all the class names?
(This is not something I'm very experienced with)
There was a problem hiding this comment.
Probably we just add V2 if we ever need to update the definition (and we can mark as experimental the first time it is released, solidifying it on the second release to give us time to figure out if needs to be changed).
| as_sedonadb_dataframe.datafusion_table_provider <- function( | ||
| x, | ||
| ..., | ||
| schema = NULL, | ||
| ctx = NULL | ||
| ) { | ||
| if (is.null(ctx)) { | ||
| ctx <- ctx() | ||
| } | ||
|
|
||
| df <- ctx$data_frame_from_table_provider(x) | ||
| new_sedonadb_dataframe(ctx, df) | ||
| } |
There was a problem hiding this comment.
Does this need to be deleted?
There was a problem hiding this comment.
Yes...there are no DataFusion implementations in R that produce this (we were the only one, and I removed the code that produced it).
|
|
||
| // Check for cancellation before fetching the next batch | ||
| // (periodic checking during fetch is handled by fetch_next_batch if configured) | ||
| if let Some(ref checker) = self.cancel_checker { |
There was a problem hiding this comment.
should we only check this if the periodic_check_interval is up?
| } | ||
| } | ||
|
|
||
| fn scan( |
There was a problem hiding this comment.
From our friend claude:
ImportedTableProvider::scan blocks a consumer async worker. The FFI scan call synchronously does thread::spawn + join around the producer's (potentially expensive, e.g. view planning) async scan. Consider spawn_blocking on the consumer side eventually. Also, the join()'s map_err discards the panic payload — the message is recoverable from the Box and would help debugging.
| ]; | ||
| assert_batches_eq!(expected, &batches); | ||
|
|
||
| // Execute partition 1 - needs a fresh import since we consumed the first |
There was a problem hiding this comment.
is this true? looks like we reuse the same one
james-willis
left a comment
There was a problem hiding this comment.
couple potential issues. not sure how important they are.
| struct SedonaCExecutionPlanArgs* args, | ||
| struct SedonaCExecutionPlan* out, struct SedonaCError* err); | ||
|
|
||
| /// \brief Resolve a synchronous stream for one partition from this plan |
There was a problem hiding this comment.
do we need to document that the implementation should be thread safe
| } | ||
| } | ||
|
|
||
| fn execute(&self, partition: usize) -> Result<SendableRecordBatchStream> { |
There was a problem hiding this comment.
Since this is not in a tokio runtime, operators that call tokio::spawn inside execute() will panic with "must be called from within a Tokio runtime."
| // This allows us to abort the fetch if cancellation is requested. | ||
| let cancel_check = async { | ||
| loop { | ||
| tokio::time::sleep(std::time::Duration::from_millis(100)).await; |
There was a problem hiding this comment.
Worker-side cancellation is dead on current-thread runtimes. In streaming.rs, the worker's cancel-poll uses tokio::time::sleep inside runtime.handle().block_on. Handle::block_on on a current_thread runtime does not drive the time driver unless Runtime::block_on is running on another thread, so that timer never fires — the worker stays blocked in stream.next() indefinitely on a stalled stream (thread leak; Drop only sets the flag, never unblocks it). The reader-side recv_timeout path masks this (and is why the tests pass — note the tests themselves dodge this with std::thread::sleep and a comment acknowledging the problem). Suggest documenting that producers should supply multi-thread runtimes, or restructuring the cancel-poll to not depend on the runtime's timer.
This PR adds an FFI wrapper around a TableProvider and enough infrastructure to replace an actual use case that we can test, which is importing a
DataFramefrom a separate SedonaDB context.